Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clean up stale augment-vis saved objs #4059

Merged

Conversation

ohltyler
Copy link
Member

@ohltyler ohltyler commented May 17, 2023

Description

This PR adds cleanup of stale/outdated/dangling augment-vis saved objs.

These objs can become "stale" if either (1) the associated (referenced) visualization has been deleted, or (2) the associated plugin resource has been deleted.

Cleaning up after deleted plugin resources

In the render workflow of a visualization, when fetching all of it's augment-vis saved objs, if any include a deleted plugin resource, delete it. See the new helper fn cleanupStaleObjects(). DEMO: see Figure 1 below.

Cleaning up after deleted visualizations

We update the vis saved object loader to implement a custom delete() fn which will fetch all augment-vis saved objs that have this vis id and delete it. To make sure this loader function is called, we update a few places where deletion of a visualization may commonly occur:

  1. Visualize plugin - table page. DEMO: see Figure 2 below.
  2. Saved objects management plugin: In the 'Object View' page (when inspecting a saved object of visualization type). DEMO: see Figure 3 below.
  3. Saved objects management plugin: table page TODO: this may not be done as part of this effort. A lot of edge cases and batch processing scenarios we need to consider. Is also more of an edge case

Additional info

  1. This also adds a RESOURCE_DELETED error type such that plugins can propagate this information to indicate their resource has been deleted. This is required in order to be picked up in cleanupStaleObjects() to get cleaned up.
  2. Plugins may also add logic of their own to clean up any dangling objects themselves at the time of plugin resource deletion. The purpose of adding deletion logic here is to take the burden off of plugins, and let cleanup happen automatically upon each fetch.

Demo videos

Figure 1: Given 2 stale augment-vis saved objects with deleted plugin resources, when re-rendering the visualization that is associated 2 these 2 objects, they are cleaned up and deleted in the background. Confirmed via .kibana index
stale-plugin-resource-demo.webm

Figure 2: Given a visualization with 1 augment-vis saved object, when deleting the visualization from visualize plugin, the augment-vis saved object is cleaned up in the background. Confirmed via .kibana index
stale-vis-visualize-plugin-demo.webm

Figure 3: Given a visualization with 1 augment-vis saved object, when deleting the visualization from saved objects management plugin's edit object page, the augment-vis saved object is cleaned up in the background. Confirmed via .kibana index
stale-vis-saved-obj-edit-demo.webm


Issues Resolved

Closes #4044

Testing the changes

Check List

  • All tests pass
    • yarn test:jest
    • yarn test:jest_integration
    • yarn test:ftr
  • New functionality includes testing.
  • New functionality has been documented.
  • Update CHANGELOG.md
  • Commits are signed per the DCO using --signoff

@ohltyler
Copy link
Member Author

ohltyler commented May 26, 2023

Thanks for the detailed proposal @ashwin-pc . Agreed this is a better strategy and removes the side-effect way of using the augment vis loader within the visualization loader's delete(). I've finished making those changes you suggested - I had to take a dependency on uiActions in a few places, but I figure that's for the best anyways so it can be used in the future.

I also wonder if uiActions belongs in core, because it seems to look that way...

Copy link
Member

@joshuarrrr joshuarrrr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks mostly good - thanks @ashwin-pc for the suggestion, I like this approach, and I agree that we may want to make UIActions even more accessible via core in the future.

I do want to verify the ability to delete non-visualization objects like visbuilder. Other than that, the only other blocker is the commented out sections.

@@ -32,7 +32,7 @@ import { SavedObjectLoader } from '../../../saved_objects/public';

export interface SavedObjectsManagementServiceRegistryEntry {
id: string;
service: SavedObjectLoader;
service: SavedObjectLoader | any;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does unknown also work here?

src/plugins/vis_augmenter/public/ui_actions_bootstrap.ts Outdated Show resolved Hide resolved
@joshuarrrr
Copy link
Member

@ohltyler There's also one failing unit test:

 FAIL  src/plugins/saved_objects_management/public/plugin.test.ts
  ● SavedObjectsManagementPlugin › #setup › registers the saved_objects feature to the home plugin

    TypeError: Cannot read property 'registerTrigger' of undefined

      19 |
      20 | export const bootstrap = (uiActions: UiActionsSetup) => {
    > 21 |   uiActions.registerTrigger(savedObjectDeleteTrigger);
         |             ^
      22 | };
      23 |

      at bootstrap (src/plugins/saved_objects_management/public/ui_actions_bootstrap.ts:21:13)
      at SavedObjectsManagementPlugin.setup (src/plugins/saved_objects_management/public/plugin.ts:145:5)
      at Object.<anonymous> (src/plugins/saved_objects_management/public/plugin.test.ts:52:20)


@ohltyler
Copy link
Member Author

@joshuarrrr addressed comments, fixed test.

src/plugins/saved_objects_management/public/plugin.ts Outdated Show resolved Hide resolved
@@ -32,7 +32,7 @@ import { SavedObjectLoader } from '../../../saved_objects/public';

export interface SavedObjectsManagementServiceRegistryEntry {
id: string;
service: SavedObjectLoader;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above, Why do we need this?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed

ohltyler added 12 commits May 30, 2023 17:39
Signed-off-by: Tyler Ohlsen <ohltyler@amazon.com>
Signed-off-by: Tyler Ohlsen <ohltyler@amazon.com>
Signed-off-by: Tyler Ohlsen <ohltyler@amazon.com>
Signed-off-by: Tyler Ohlsen <ohltyler@amazon.com>
Signed-off-by: Tyler Ohlsen <ohltyler@amazon.com>
Signed-off-by: Tyler Ohlsen <ohltyler@amazon.com>
Signed-off-by: Tyler Ohlsen <ohltyler@amazon.com>
Signed-off-by: Tyler Ohlsen <ohltyler@amazon.com>
Signed-off-by: Tyler Ohlsen <ohltyler@amazon.com>
Signed-off-by: Tyler Ohlsen <ohltyler@amazon.com>
Signed-off-by: Tyler Ohlsen <ohltyler@amazon.com>
Signed-off-by: Tyler Ohlsen <ohltyler@amazon.com>
@ohltyler ohltyler force-pushed the saved-obj-cleanup branch from 34c2a21 to 67ae9ab Compare May 31, 2023 01:06
Signed-off-by: Tyler Ohlsen <ohltyler@amazon.com>
ohltyler added 2 commits May 31, 2023 08:40
Signed-off-by: Tyler Ohlsen <ohltyler@amazon.com>
Signed-off-by: Tyler Ohlsen <ohltyler@amazon.com>
Copy link
Member

@ashwin-pc ashwin-pc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perfect! thanks for making those changes :)

@ohltyler ohltyler merged commit 538d91e into opensearch-project:feature/feature-anywhere May 31, 2023
@ohltyler ohltyler deleted the saved-obj-cleanup branch May 31, 2023 17:04
@kavilla kavilla added v2.9.0 and removed v2.8.0 labels Jun 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants